-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Adding a blur graphical primitive and use it under modal popups & dialogs #5849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
51efddc to
4c43069
Compare
| // If it is a master painter it will also initialise OpenGL | ||
| func NewPainter(c fyne.Canvas, ctx driver.WithContext) Painter { | ||
| p := &painter{canvas: c, contextProvider: ctx} | ||
| p.blurKernels = make(map[float32][]float32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to cache the kernel on the canvas.Blur itself? With a map like this we need a clean task / expiry too, and it seems that caching it on the blur object would avoid the overhead of a map lookup (which, granted, is small). You can have
package "canvas"
type Blur struct {
// ... public fields
cachedKernelRadius float32
cachedKernel []float32
}Then you can know whether the kernel needs to rebuilt for a new radius (if the .Radius property on the blur has been updated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it is a detail of the renderer not the blur. and a 5pt kernel is the same for all blurs - seemed like the cache would be smaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all the ones the theme applies to dialog backgrounds, yes. But user code could do something like animate a blur over a range of radii, causing a potentially unbounded number of entries into the blurKernels cache. We can keep it a map, but we'll have to make sure we have a system to delete from it, like all caches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option to cache on the canvas.Blur but to more closely follow this
Well, it is a detail of the renderer not the blur.
would be to create a field on the Blur struct: painterData any, that whichever loaded painter is allowed to use, so the logic to generate the kernel would be kept in the gl painter, and it would type-assert that field to its own blurKernel struct or whatever; it just is allowed to tack it onto the Blur object itself instead of needing a map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the caching here makes more sense like it is now. Breaking the barrier between objects and internal details seems undesirable. However, I do think that the potentially unbounded map growth is a problem that needs to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth noting that this issue would apply here as well - but I had a draft PR to address this (which I really should pick up again sometime) and can add the blurKernel cache to this new system as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bear in mind the kernel cache is a OpenGL requirement not all renderers.
Can you expand on why caching a rendering lookup table on the object is clearer?
Also I'm unsure how a private field on a struct in a different package is even something the renderer code can reference... I feel I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'm unsure how a private field on a struct in a different package is even something the renderer code can reference...
Oh, of course you're right about that 🤦
My other original thought was that computing the kernel itself doesn't seem expensive compared to the allocation (just a pair of non-nested loops over what usually will be like 8 iterations only). Since we're single threaded now and only one blur will ever be painted at a time, even a static []float32 slice declared in the gl package that the painter can reset the length to zero (NOT setting it to nil) and re-fill the same memory buffer with a freshly-computed kernel for each paint would probably be viable.
Same idea applies for keeping a static []uint8 slice that can be zeroed out and reused to capture the pixel data under the blur on each paint, for the other allocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not getting why we would choose to re-compute every kernel for every frame over using some memory to cache them...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use memory to cache them but we must add the cache clean task then, so that we don't retain that memory indefinitely for every blur radius ever asked for by the app. Right now we are only ever putting things into the blurKernels map and never deleting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Really cool 👍
| // If it is a master painter it will also initialise OpenGL | ||
| func NewPainter(c fyne.Canvas, ctx driver.WithContext) Painter { | ||
| p := &painter{canvas: c, contextProvider: ctx} | ||
| p.blurKernels = make(map[float32][]float32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the caching here makes more sense like it is now. Breaking the barrier between objects and internal details seems undesirable. However, I do think that the potentially unbounded map growth is a problem that needs to be fixed.
|
Nitpick but the checklist item for binary size increase when importing module should probably be checked :) |
|
iOS is good but performance is not good enough IMHO. |
Tested on:
Checklist: